bootstrap: move policy setup code to internal/process/policy.js #43282
Conversation
To keep pre_execution.js focusing on bootstrap instead of domain-specific logic.
|
Review requested:
|
nodejs-github-bot
added
needs-ci
| // no bare specifiers for now | ||
| let manifestURLObject; | ||
| if (isAbsolute(policyPath)) { | ||
| manifestURLObject = new URL(`file://${policyPath}`); |
There was a problem hiding this comment.
What if the path contains a char that has a special meaning in URL (e.g. ?)? @watilde since you're the one who committed this line, do you know if that should be that instead:
| manifestURLObject = new URL(`file://${policyPath}`); | |
| manifestURLObject = pathToFileUrl(policyPath); |
(non-blocking for this PR)
There was a problem hiding this comment.
I'd rather leave this to other PRs as this one only tries to move things to a different palce (same with other suggestions below, but I can apply the first ones since they are only added in this PR).
| const cwdURL = pathToFileURL(process.cwd()); | ||
| cwdURL.pathname += '/'; | ||
| manifestURLObject = new URL(policyPath, cwdURL); |
There was a problem hiding this comment.
Same here, I think it should be:
| const cwdURL = pathToFileURL(process.cwd()); | |
| cwdURL.pathname += '/'; | |
| manifestURLObject = new URL(policyPath, cwdURL); | |
| manifestURLObject = pathToFileURL(join(process.cwd(), policyPath)); |
| for (let i = 0; i < integrityEntries.length; i++) { | ||
| const { | ||
| algorithm, | ||
| value: expected |
There was a problem hiding this comment.
| value: expected | |
| value: expected, |
|
Looks like the CI failures are genuine, there are some TDZ problems with the worker code path |
|
I'm sure you've heard, but the policy model is being deprecated and removed. |
To keep pre_execution.js focusing on bootstrap instead of
domain-specific logic.